Skip to content

Feat/implement phase d#3

Merged
nixrajput merged 11 commits into
mainfrom
feat/implement-phase-d
May 31, 2026
Merged

Feat/implement phase d#3
nixrajput merged 11 commits into
mainfrom
feat/implement-phase-d

Conversation

@nixrajput

@nixrajput nixrajput commented May 31, 2026

Copy link
Copy Markdown
Owner

Summary by CodeRabbit

Release Notes

  • New Features

    • Driver capability gating: Parallel backups and native stream sync now verify driver support before execution.
    • Enhanced Postgres connection reliability with automatic retry logic for unavailable servers.
  • Documentation

    • Added Pre-1.0 development warning to README.
    • New contributor guide for implementing database drivers.
    • Updated changelog documenting Phase B–D features (backup/restore, dashboard, driver hardening).
  • Tests

    • Added comprehensive driver integration test suite for standardized contract validation.

nixrajput added 9 commits May 31, 2026 17:07
Add RequireCapability(d Deps, profileName string, requiredCap Capability)
in the app layer. It resolves the profile to its driver internally and
checks the driver's Capabilities, returning a CodeUser errs.Error wrapping
ErrDriverUnsupported when the capability is missing.

Taking a profile name (not a driver.Driver) keeps the CLI/TUI from having
to import internal/driver, respecting the depguard boundary.
Probe the connection via jobs.Retry (spec §4.3: 3 attempts with
exponential backoff) so a briefly-unavailable server is not a hard
fail. sql.Open stays a single lazy call; only PingContext is retried.
On final failure the error still maps through wrapConnErr to
errs.ErrConnectionFailed.

Move Retry from internal/app to internal/jobs: the driver may not
import internal/app (depguard driver-may-not-import-presentation), and
driver already imports internal/jobs, so this avoids the import cycle.
Retry had no callers; add a unit test for it in the jobs package.
@nixrajput nixrajput self-assigned this May 31, 2026
@coderabbitai

coderabbitai Bot commented May 31, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@nixrajput, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 21 minutes and 58 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 456f1013-8106-4ad9-872c-9679798ee396

📥 Commits

Reviewing files that changed from the base of the PR and between c2a8047 and 15e24dd.

📒 Files selected for processing (11)
  • CHANGELOG.md
  • README.md
  • docs/DRIVERS.md
  • internal/app/capgate.go
  • internal/app/capgate_test.go
  • internal/driver/_testing/fixtures.go
  • internal/driver/_testing/suite.go
  • internal/driver/postgres/driver.go
  • internal/driver/postgres/integration_test.go
  • internal/jobs/retry.go
  • internal/jobs/retry_test.go
📝 Walkthrough

Walkthrough

This PR implements Phase D driver-layer hardening by introducing capability gating to control driver feature availability, a shared driver contract-test harness for validating driver implementations, relocating the Retry utility to a jobs package, adding Postgres connection-retry behavior, and integrating capability checks into CLI commands (backup/sync). New docs/DRIVERS.md guide provides comprehensive driver implementation requirements.

Changes

Driver Hardening and Testing Infrastructure

Layer / File(s) Summary
Documentation and Changelog
CHANGELOG.md, README.md, docs/DRIVERS.md
Announces Phase D hardening with driver test harness, capability gating, Postgres retry, Retry relocation, and new driver guide. Expands project status table and links driver implementation docs.
Retry helper relocation and unit tests
internal/jobs/retry.go, internal/jobs/retry_test.go
Moves Retry function from app to jobs package and adds unit tests for immediate success, transient-failure retry path, exhaustion, and context cancellation behaviors.
Capability gating layer and tests
internal/app/capgate.go, internal/app/capgate_test.go
Defines Capability type, 13 Cap* constants, and RequireCapability function to resolve profiles and validate driver capability support. Tests verify success on supported capabilities, error wrapping on unsupported, and mapping correctness.
Driver testing infrastructure
internal/driver/_testing/fixtures.go, internal/driver/_testing/suite.go
Adds Fixtures struct for test database lifecycle and RunDriverSuite helper that exercises driver contract via subtests: connection/inspection, backup-seed-restore cycle, cancellation propagation, and bad-credential error mapping.
Postgres driver retry and suite integration
internal/driver/postgres/driver.go, internal/driver/postgres/integration_test.go
Updates Postgres Connect to retry initial database ping via jobs.Retry. Replaces standalone tests with suite-based tests providing profile, cleanup, SQL opener, Seed, and VerifyRestore fixtures.
CLI command capability wiring
internal/cli/backup.go, internal/cli/sync.go
Backup requires CapParallel when --jobs > 1. Sync requires CapNativeStream for both source and target when --stream is enabled. Validation runs before operation execution.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • nixrajput/siphon#1: Introduces initial Postgres driver PingContext-based connection validation; this PR enhances that with retry-on-transient-failure behavior via jobs.Retry.

Poem

🐰 Hark! The drivers now can declare their gifts,
With capabilities gating each CLI shift.
A shared test harness ensures all contracts hold,
And Postgres retries—no failures! How bold!
From phase to phase, the system takes flight.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The PR title 'Feat/implement phase d' is vague and generic, using an unclear reference to 'phase d' without explaining what features or changes are actually being implemented. Revise the title to be more descriptive. Examples: 'Add driver capability gating and contract-test harness' or 'Implement Phase D: driver hardening with capability gates and test suite'.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/implement-phase-d

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/DRIVERS.md`:
- Around line 27-35: The fenced code block containing the ASCII architecture
diagram that starts with "internal/cli   internal/tui     presentation (Cobra ·
Bubble Tea)" is missing a language tag and triggers markdownlint MD040; update
that triple-backtick fence to include a language identifier (e.g., change ``` to
```text) so the block becomes a text code block and the MD040 warning is
resolved.

In `@internal/cli/sync.go`:
- Around line 30-36: The CLI currently defaults --stream to true and
preflight-checks app.CapNativeStream (in the block referencing deps, fromName,
toName), which mismatches internal/app/sync.go where opt.Stream is ignored and
Sync always uses io.Pipe; to fix, change the CLI so it does not require
NativeStream by default: set the --stream flag default to false (or remove the
preflight check), and only call app.RequireCapability(deps, name,
app.CapNativeStream) when you actually intend to use native driver streaming
(i.e., when opt.Stream is true AND the code path will invoke the driver's native
stream API); reference opt.Stream, Sync in internal/app/sync.go, and
app.CapNativeStream/app.RequireCapability when making the conditional.

In `@internal/driver/_testing/suite.go`:
- Around line 81-104: The Cancel_PropagatesToSubprocess test
(Cancel_PropagatesToSubprocess) assumes conn.Backup spawns a long-running
pg_dump and cancels after 150ms, but a small dataset from
BackupRestore_Roundtrip can let pg_dump finish early; to fix, make the subtest
create a larger dump workload before starting conn.Backup (e.g., insert/seed
many more rows or otherwise inflate the DB in this subtest) so pg_dump is almost
certainly in-flight when cancel() is called, or alternatively relax the
assertion after the cancel to accept a nil error if the subprocess already
finished; update the Cancel_PropagatesToSubprocess test code around the
conn.Backup call to perform the seeding (or to change the post-cancel error
check) so it reliably exercises cancellation.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bda4beb6-f53d-4b6c-8395-5e0ddd19845e

📥 Commits

Reviewing files that changed from the base of the PR and between 66a5d07 and c2a8047.

📒 Files selected for processing (13)
  • CHANGELOG.md
  • README.md
  • docs/DRIVERS.md
  • internal/app/capgate.go
  • internal/app/capgate_test.go
  • internal/cli/backup.go
  • internal/cli/sync.go
  • internal/driver/_testing/fixtures.go
  • internal/driver/_testing/suite.go
  • internal/driver/postgres/driver.go
  • internal/driver/postgres/integration_test.go
  • internal/jobs/retry.go
  • internal/jobs/retry_test.go

Comment thread docs/DRIVERS.md Outdated
Comment thread internal/cli/sync.go Outdated
Comment thread internal/driver/_testing/suite.go
Remove the `backup --jobs`→CapParallel and `sync --stream`→CapNativeStream
preflight gates. Both guarded features that the current verbs do not yet
implement: app.Sync ignores opt.Stream (always pipes Backup→Restore via
io.Pipe), and pg_dump parallelism (-j/-Fd) is unwired ("not yet effective;
Phase F"). Gating them now would falsely reject valid operations the moment a
driver declaring NativeStream:false / Parallel:false lands, while gating a
no-op feature. The RequireCapability helper + Capabilities flags stay; the
gates get wired in Phase F alongside the features they guard.

Also from CodeRabbit review on PR #3:
- _testing/suite.go: the Cancel_PropagatesToSubprocess subtest could flake on
  fast hosts when the prior subtest's tiny fixture lets pg_dump finish before
  the 150ms cancel. Seed ~5 MiB via fx.SQLOpener so the dump is reliably
  in-flight at cancel time; keep the strong non-nil assertion. (Integration-tag
  only; not run in CI.)
- DRIVERS.md: add a `text` language tag to the architecture-diagram fence
  (markdownlint MD040), and correct the capability section + README/CHANGELOG
  to describe gating as helper-available-but-not-yet-wired, not CLI-wired.
nixrajput added a commit that referenced this pull request May 31, 2026
Remove the `backup --jobs`→CapParallel and `sync --stream`→CapNativeStream
preflight gates. Both guarded features that the current verbs do not yet
implement: app.Sync ignores opt.Stream (always pipes Backup→Restore via
io.Pipe), and pg_dump parallelism (-j/-Fd) is unwired ("not yet effective;
Phase F"). Gating them now would falsely reject valid operations the moment a
driver declaring NativeStream:false / Parallel:false lands, while gating a
no-op feature. The RequireCapability helper + Capabilities flags stay; the
gates get wired in Phase F alongside the features they guard.

Also from CodeRabbit review on PR #3:
- _testing/suite.go: the Cancel_PropagatesToSubprocess subtest could flake on
  fast hosts when the prior subtest's tiny fixture lets pg_dump finish before
  the 150ms cancel. Seed ~5 MiB via fx.SQLOpener so the dump is reliably
  in-flight at cancel time; keep the strong non-nil assertion. (Integration-tag
  only; not run in CI.)
- DRIVERS.md: add a `text` language tag to the architecture-diagram fence
  (markdownlint MD040), and correct the capability section + README/CHANGELOG
  to describe gating as helper-available-but-not-yet-wired, not CLI-wired.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@nixrajput nixrajput force-pushed the feat/implement-phase-d branch from 5831efe to 15e24dd Compare May 31, 2026 13:38
@nixrajput nixrajput merged commit 4099cee into main May 31, 2026
5 checks passed
@nixrajput nixrajput deleted the feat/implement-phase-d branch May 31, 2026 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant